implemented har feature#7970
Conversation
|
@kenzieschmoll @exaby73 |
bkonyi
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I've got mostly minor comments so far.
bkonyi
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments!
| }; | ||
| }).toList(); | ||
|
|
||
| // Assemble the final HAR object |
There was a problem hiding this comment.
sorry didn't get you
| static const onContentLoad = -1; | ||
| static const onLoad = -1; | ||
| static const httpVersion = 'HTTP/1.1'; | ||
| static const responseHttpVersion = 'http/2.0'; |
There was a problem hiding this comment.
this field doesn't seem to be available in the 'DartIOHttpRequestData'
There was a problem hiding this comment.
Can we put some placeholder like "Unknown" instead? Maybe @brianquinlan knows if this information is available somewhere.
|
@kenzieschmoll as I observed the offline functionality works in these ways :
I have implemented the 1st one (changes yet to be pushed as of now). |
|
@bkonyi can you re-check the comments and mark them as resolved if they are addressed |
Sorry for the delay, I'll take a look. |
|
All of my comments are basically addressed and this LGTM overall. I'll let @kenzieschmoll give final say on the PR. Thanks for your contribution! |
Looking forward to contributing more in future, after this one is done! |
kenzieschmoll
left a comment
There was a problem hiding this comment.
I apologize, I had a pending review that I never hit submit on. Here are my recommendations.
kenzieschmoll
left a comment
There was a problem hiding this comment.
LGTM once all tests pass. Thanks for the feature!
Thank you for the review! I'll ensure all tests pass. Appreciate the feedback and support! |
@kenzieschmoll All the tests have passed can this be merged now? Or Anything else remaining? |
This PR addresses below issue -
#3806
Please add a note to
packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.mdif your change requires release notes. Otherwise, add the 'release-notes-not-required' label to the PR.Pre-launch Checklist
///).If you need help, consider asking for help on Discord.